Fix MastForest serialization
#1482
Conversation
plafer
added
the
no changelog
There was a problem hiding this comment.
Thank you! Not a full review but I've left some comments inline.
Also, maybe I missed it, but I think the current code doesn't actually serialize/deserialize decorators for non-basic block nodes.
For this, we'd need to include before/after decorator offsets to MastNodeInfo like so:
pub struct MastNodeInfo {
ty: MastNodeType,
digest: RpoDigest,
before_decorators_offset: u32,
after_decorators_offset: u32,
}But then these new fields would not be really used for basic block nodes (though, maybe we can still use them and get rid of decorator_list_offset field for MastNodeType::Block).
Another thing to consider is that the formats of decorator lists for basic blocks and other nodes are different - i.e., in one case it would be just a list of DecoratorIds and in the other it would be a list of (usize, DecoratorId) tuples. But maybe this doesn't matter and we can make the data context-dependent.
Correct, this is still left to be done in this PR.
This would add 8 bytes to
Upon deserialization, after deserializing all |
Ah yes - that's a better solution! |
There was a problem hiding this comment.
Looks good! Thank you! I left some small comments inline, once these are addressed, we can merge.
Also, I'm curious how this change impacted both deserialization time and execution time (I would think that the impacts should be negligible, but would be good to confirm).
|
For the blake3 benchmark (100 iterations), both |
* feat: move decorators to `MastForest` * `DecoratorList` uses `DecoratorId` * feat: add `before_enter` and `after_exit` decorators to MAST nodes * WIP: `MastForestBuilder::make_basic_block()` returns `Either<Option<MastNodeId>, Vec<DecoratorId>>` Still WIP on fixing `compile_body()` - specifically handling what to do when `make_basic_block()` returns a list of decorators * fix build * fix build after merge * fix repeat statement * implement `before_enter` and `after_exit` * changelog * fix no_std * print decorators in BasicBlockNode * join node pretty print * basic block and repeat tests * join decorators test * implement pretty print for all nodes * call node pretty print * dyn node pretty print * decorators external node * join and split node pretty print * fix `eq_hash`, and test `decorators_repeat_split` * `MastForestBuilder`: add_decorator -> ensure_decorator * fix asmop hash * fix docs * fmt * add failing test * eq_hash: now takes into account children's eq hashes * remove redundant comment * fix `MastForestBuilder::ensure_node` docs * expand on `add_library` comment * Remove `Either` type * `add_block_with_raw_decorators` clarifying comment * Move `&mut MastForestBuilder` into `BasicBlockBuilder` * split `add_library()` docstring into 3 paragraphs * Fix `MastForest` serialization (#1482) * Move strings data to its own buffer * move StringTableBuilder out of BasicBlockDataBuilder * Create `StringTable` type * Encode decorators separately * fix strings table * Introduce Operation iterator in BasicBlockNode * Document use of `MAX_DECORATORS` value * fix typo * merge `basic_block_data_decoder` and `basic_block_data_builder` modules * Use `DecoratorId::from_u32_safe` in deserialization * serialization failing test * serialize before_enter/after_exit maps * Document serialization format * rename `BasicBlockNode::operation_iter` * `MastNodeInfo::new()`: soft enforce 0 values for non-basic block nodes * update docstring for `BasicBlockDataDecoder` * decorator id deserializing meaningful errors * move `basic_block_data_decoder` var * Don't use maps for before enter/after exit decorators * block_builder var name * Introduce new `EqHash` * Cleanup `BasicBlockBuilder::make_block()` * Incorporate assert and mpverify arguments into eq_hash * update code organization docs
Builds on #1466.
Stores the decorators in a separate buffer during serialization.